Speed up loading file statuses in Git commit dialog by batching events and skipping events for up-to-date files#9324
Conversation
mbien
left a comment
There was a problem hiding this comment.
this looks good to me. Thanks a lot for the improvements.
left some comments inline but its nothing important
| for (ChangedEvent event : events) { | ||
| fireFileStatusChanged(event); | ||
| if (!events.isEmpty()) { | ||
| listenerSupport.firePropertyChange(PROP_FILES_STATUS_CHANGED, null, events); |
There was a problem hiding this comment.
good catch. aggregating events can certainly reduce overhead under load. Reminds me a little on #8955 where I saw 1.5mil event handler invocations when a large project group opened.
There was a problem hiding this comment.
Actually, the overhead reduction is almost negligible. I was tempted to remove this but then I found out one more way to optimize this, by running the property change listeners asynchronously in background thread.
Each single file status update is an I/O operation and thus slow. Running the updates in background moves this off the main thread. Running everything in batch allows offloading everything to a single background thread instead of updating each file in separate thread, which might be slower because of synchronization between threads.
Therefore I kept the property change event for all of the files and changed the 2 listeners to run in background, updating all the files with a single background thread.
| fireStatusChanged(changedEvent.getFile()); | ||
| } else if (event.getPropertyName().equals(FileStatusCache.PROP_FILES_STATUS_CHANGED)) { | ||
| List<FileStatusCache.ChangedEvent> changedEvents = (List<FileStatusCache.ChangedEvent>) event.getNewValue(); | ||
| Set<File> files = new HashSet<>(changedEvents.size()); |
There was a problem hiding this comment.
this is super nitpicky. but the constructor arg isn't for the element count, it is to size the internal table - which sometimes leads to initial sizes which are too small.
if you want you can bump the module to javac.release=21 (see project.properties) and then use the HashSet.newHashSet(numElements) factory which does some math before setting the size of the internal table.
feel free to update the commit and force push in place.
There was a problem hiding this comment.
Updated as suggested
| for (FileStatusCache.ChangedEvent changedEvent : changedEvents) { | ||
| if (revisionLeft == Revision.HEAD // remove when we're able to refresh single file changes for Local vs. any revision | ||
| && revisionRight == Revision.LOCAL && affectsView(changedEvent)) { |
There was a problem hiding this comment.
could the revisionLeft/Right check be moved before the loop?
There was a problem hiding this comment.
Updated as suggested
94bdb4a to
e48d9d6
Compare
|
@OndroMih for some reason I see an extra commit in this PR branch.
|
|
@mbien , I implemented the changes you requested, please review again. I added one more optimization - property change listeners run asynchronously - they both do a slow I/O operation for each file and running them asynchronously unblocks the main thread. This doesn't have impact on functionality because the final stage of updating the statuses was already asynchronous and it only updates the icons of files to reflect they status. Here's a sceenshot from profile before the change - the After these changes, when I forced update also on noupdate files, this decreased to 1 second: When I put back skipping update on noupdate files, the |
e48d9d6 to
2f76482
Compare
| final RequestProcessor.Task refreshAnnotationsTask = rp.create(() -> { | ||
|
|
||
| if (files == null) { | ||
| LOG.log(Level.FINE, "refreshing all annotations"); //NOI18N | ||
| refreshAllAnnotationsTask.schedule(2000); | ||
| return; | ||
| } |
There was a problem hiding this comment.
first if check could be before the task in the refreshAnnotations() method.
## Skip firing events for up-to-date Skip firing events for up-to-date files that are not yet in the cache, since UPTODATE is the default for managed files. This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms. ## Batch status change notifications Batch status change notifications to avoid per-file event overhead. Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k redundant propertyChange/schedule/SwingUtilities.invokeLater calls on first load. This improves performance a bit because it eliminates many method calls. However, in the end, the number of files changed is the same so the event handler still needs to process all of them. ## Move status updates to a background thread File status update requires I/O operation to refresh files metadata from FS. This is very slow when many files need to be updated. Moving them to a background thread offloads this slow operation from the main thread. Updates can run asynchronously without blocking the main thread that fires the status events, they just update UI hints, they have no impact no behavior. For the whole Netbeans repository, this shortens the time it takes to complete the firePropertyChange event from 6 seconds to 1 second.
2f76482 to
642c71b
Compare


Skip firing events for up-to-date
Skip firing events for up-to-date files that are not
yet in the cache, since UPTODATE is the default for managed files.
This drastically reduces the time spent in the refreshStatusesBatch method on big repositories executed when Commit dialog opens. For example, on the Netbeans repository, from around 8 seconds to 20ms.
Batch status change notifications
Batch status change notifications to avoid per-file event overhead.
Replace per-file PROP_FILE_STATUS_CHANGED firing in refreshStatusesBatch
with a single PROP_FILES_STATUS_CHANGED batch event, eliminating 100k
redundant propertyChange/schedule/SwingUtilities.invokeLater calls on
first load. This improves performance a bit because it eliminates many method calls.
However, in the end, the number of files changed is the same so the event handler
still needs to process all of them.
Move status updates to a background thread
File status update requires I/O operation to refresh files metadata from FS. This is very slow when many files need to be updated. Moving them to a background thread offloads this slow operation from the main thread.
Updates can run asynchrnously without blocking the main thread that fires the status events, they just update UI hints, they have no impact no behavior.
For the whole Netbeans repository, this shortens the time it takes to complete the firePropertyChange event from 6 seconds to 1 second.
Complements #9304, which speeds up the process even more, in a different area.
Click to collapse/expand PR instructions
PR approval and merge checklist:
If this PR targets the delivery branch: don't merge. (full wiki article)